feat: complete range repartition physical planning#23617
Conversation
|
cc @gene-bordegaray & @gabotechs ! Thanks in advance for any feedback! 😁 🎉 |
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
left some feedback, thank you 🙇
|
@gene-bordegaray thanks for the feedback! Pushed a commit to address - appreciate all the time taken reviewing! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23617 +/- ##
==========================================
- Coverage 80.67% 80.66% -0.01%
==========================================
Files 1086 1086
Lines 366800 367022 +222
Branches 366800 367022 +222
==========================================
+ Hits 295912 296073 +161
- Misses 53260 53294 +34
- Partials 17628 17655 +27 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
Thanks for updating this, I think my earlier comment may have pushed this a little too far in the helper direction 😅
What I was trying to get at was mostly reducing the repeated range-partitioning setup, not adding a bunch of extra test scaffolding / coverage. I think we can keep this much tighter:
- one positive projection-swap test that verifies the range key is rewritten when its projected column index changes
- one negative projection-swap test that covers dropping a single key and dropping part of a compound key
- two sort-pushdown tests: supported when the range repartition maintains input order, unsupported when it does not
- keep the repartitioned() None test
I’d drop the extra execution sortedness test since this PR really about the planning behavior.
For helpers I think the useful ones are only the small range-specific setup things like build range partitioning from cols, build a projection from cols, unwrap/assert range partitioning.
Haha no worries, my bad - updated! Thanks again for reviewing :) |
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
looking much better 🚀
Addressed! 🙌 |
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
looks good now 👍 thank you @saadtajwar
ready for you now @gabotechs 🙇
| ); | ||
| assert!(repartition.maintains_input_order()[0]); | ||
|
|
||
| match repartition.try_pushdown_sort(ordering.as_ref())? { |
There was a problem hiding this comment.
non-blocking: maybe one last assert comparing the sort of the pushed repartiton and required 👍
3 participants
Which issue does this PR close?
Rationale for this change
After #23231 was merged in for supporting physical execution of the range repartitioning scheme, we still had a few methods on physical planning unimplemented, specifically
try_swapping_with_projection,try_pushdown_sort,repartitioned- this PR finishes the implementation of those methodsWhat changes are included in this PR?
try_swapping_with_projection: similar to theHashscheme, forRangewe callupdate_exprfor each of the range key expressions to attempt rewriting based on the projection expressionstry_pushdown_sort: same as other variants, we delegate to the child and wrap with a newRepartitionExecrepartitioned: unable to support for Range, left comment in codebase with explanationAre these changes tested?
Yes
Are there any user-facing changes?
No